141882981-Add-question-type-to-content-loader-and-summary-display #36

benvand · 2017-04-04T11:23:07Z

Create handlers for processing and displaying date questions.

Create a new Date class for handling date questions.
Create a new DateSummary class for handling viewing dates in summary tables etc.
Register Date handler in QUESTION_TYPES to make it auto used by all date type questions.
Tests.

https://trello.com/c/G3J4fbtw/30-add-question-type-to-content-loader-and-summary-display

Add new interfaces for `date` type questions (3 part d, m, y inputs). * `dmcontent/content_loader.py` * Extrapolate out assurance unformatting into new method * Add method to unformat dates * Add `_is_date` method on content to find out if a question is a date * `dmcontent/questions.py` * Add property `is_date` to question * Add `Question` class `Date` as interface for date questions * Add `DateSummary` `QuestionSummary` class to present date in given format * Make `date` type questions available in `QUESTION_TYPES` * Handle old date strings in date summary

idavidmcdonald

It feels like unformat_data for a date question should be on the DateQuestion itself. Similar to how we do for dynamic lists (https://github.com/alphagov/digitalmarketplace-content-loader/blob/master/dmcontent/questions.py#L369) and all question types (https://github.com/alphagov/digitalmarketplace-content-loader/blob/master/dmcontent/questions.py#L114).

However the reason this works (from a quick glance around one of the frontend apps) is because we are calling unformat_data directly on the question in our views (https://github.com/alphagov/digitalmarketplace-supplier-frontend/blob/1bc8418226e15ba6a92f4cff7c25c23dbcf8ddf6/app/main/views/briefs.py#L202) rather than using the content section unformat_data.

Maybe we should be trying to mimic the approach of get_data (https://github.com/alphagov/digitalmarketplace-content-loader/blob/master/dmcontent/content_loader.py#L64) where when getting data for a section we run through every section and then every question letting each question then be responsible for getting it's own data. Doing it that way would feel more object orientated than the current solution. Ideally we wouldn't add a new 'if' statement every time unformatting data is different for a different type of question.

Note, the reason assurance appears to be an exception is because we do not have an AssuranceQuestion, but maybe this could be restructured similarly to be on on question types that can have assurance rather than a content section.

What do you think (especially if I haven't spotted something or my suggestion isn't possible/sensible)?

idavidmcdonald · 2017-04-05T10:13:18Z

dmcontent/questions.py

+    @property
+    def value(self):
+        try:
+            return datetime.strptime(self._value, '%Y-%m-%d').strftime('%A %-d %B %Y')


If we are formatting it into the common digital marketplace format we would ideally use the dmutils DATE_FORMAT instead of duplicating

There's a reason we we've been avoiding that. Has to do with having to add it as a dependency link in the setup.py as digitalmarketplace-content-loader is a module. I've added it, we can chat on Friday.

idavidmcdonald · 2017-04-05T10:20:32Z

tests/test_questions.py

+        return ContentQuestion(data)
+
+    def test_date_is_formatted_into_user_friendly_format(self):
+        question = self.question().summary({'example': '2016-02-18'})


Is it worth having a test for where we don't include 0 padding i.e. 2?

Either we are saving the month input directly in which case we may see data like {'example': '2017-2-19'} in our DB, and therefore we need to know our DateSummary is able to handle that or we only save data that includes 0 padding.

Cool, the test below now addresses this.

idavidmcdonald · 2017-04-05T10:26:16Z

dmcontent/questions.py

+        """Retreive the fields from the POST data (form_data).
+
+        The d, m, y should be in the post as 'questionName-day', questionName-month ...
+        Extract them and format as 'YYYY-MM-DD'.


I guess technically we can't gurantee it will be 'YYYY-MM-DD' as the user input could be anything such as 'YYYY-M-D'. This is a really anal comment though...

idavidmcdonald · 2017-04-05T10:44:38Z

dmcontent/content_loader.py

    def unformat_data(self, data):
+
        """Unpack assurance information to be used in a form


If we do decide to stick with this solution then documentation for this function needs updating. Also is it worth adding a test to cover unformat_date and maybe calling unformat_data when passing a date field in?

idavidmcdonald · 2017-04-05T10:46:33Z

dmcontent/questions.py

+        try:
+            return datetime.strptime(self._value, '%Y-%m-%d').strftime('%A %-d %B %Y')
+        except ValueError:
+            return self._value


Could be nice to include a very short comment explaining that we are falling back to original date format

idavidmcdonald · 2017-04-07T12:34:00Z

dmcontent/questions.py

@@ -182,6 +185,10 @@ def inject_brief_questions_into_boolean_list_question(self, brief):
    def has_assurance(self):
        return True if self.get('assuranceApproach') else False

+    @property


Do we still need this?

idavidmcdonald · 2017-04-07T12:34:12Z

dmcontent/questions.py

+
+    FIELDS = ('year', 'month', 'day')
+
+    @property


Ditto here, is this now redundant?

idavidmcdonald · 2017-04-07T12:39:51Z

dmcontent/questions.py

+        parts = []
+        for key in self.FIELDS:
+            identifier = '-'.join([self.id, key])
+            value = form_data.get(identifier, '').replace('-', '').strip()


I think we should have a test for the stripping - behaviour

idavidmcdonald · 2017-04-07T12:42:09Z

tests/test_questions.py

+            'example-day': '19',
+            'example-month': '03',
+            'example-year': '',
+        }) == {'example': None}


Do we expect this to be None rather than -03-19?

idavidmcdonald · 2017-04-07T12:47:26Z

dmcontent/questions.py

+
+        return {self.id: '-'.join(parts) if any(parts) else None}
+
+    def unformat_data(self, data):


Do we have any tests for this method?

idavidmcdonald · 2017-04-07T12:49:53Z

dmcontent/content_loader.py

    def unformat_data(self, data):
+
        """Unpack assurance information to be used in a form


This doc block top line doesn't feel correct anymore.

idavidmcdonald

Generally looks good.

Few comments regarding some potentially redundant things, old comments and missing test coverage. It will also need a version bump.

What is your view on using the dmutils dependancy? You seemed to imply there are some downsides to doing this?

digitalmarketplace-content-loader/tests/test_content_loader.py::TestReadYaml::test_loading_existant_file The above was failing on master, there's been a refactor recently but not sure what caused it tbh. Fixed mock to point at instance open that is used by the function being tested.

idavidmcdonald

Looking very good Ben.

Think I left just 3 minors now comments. One potential typo and one suggested comment improvement.

The only thing left is regarding the interface for unformat_data which I preferred the old way and am not quite sure if there is a reason for us to do it otherwise. Maybe you could shed some light?

idavidmcdonald · 2017-04-11T12:58:45Z

dmcontent/content_loader.py

    def unformat_data(self, data):
-        """Unpack assurance information to be used in a form
+        """Method to process form data, special assurance case or individual question level unformat.


I might suggest something including something along the lines of into data to be used in a form would be good to have in here as at the moment it reads like you are processing form data rather than processing some data that you can then pass to a form.

idavidmcdonald · 2017-04-11T13:06:44Z

dmcontent/questions.py

@@ -366,7 +369,7 @@ def get_data(self, form_data):

        return {self.id: questions_data}

-    def unformat_data(self, data):
+    def unformat_data(self, key, data):


Why do we want to have two different interfaces for unformat_data? It felt nice that unformat_data had the same interface regardless of if you were calling it on a Section or Question. That pattern also mimics the get_data usage.

idavidmcdonald · 2017-04-11T13:09:10Z

tests/test_content_loader.py

    def test_loading_existant_file(self, mocked_open):
        assert read_yaml('anything.yml') == {'foo': 'bar'}

-    @mock.patch.object(builtins, 'open', side_effect=IOError)
+    @ mock.patch('dmcontent.content_loader.open', side_effect=IOError)


Should there be a space in here?

idavidmcdonald · 2017-04-12T14:28:00Z

I'm slightly confused about what we want unformat_data to do.

Originally unformat_data would take any dictionary of data and return that data, unformatting any keys that were related to that question but everything else would be left untouched
(dynamic list questions are an example https://github.com/alphagov/digitalmarketplace-content-loader/blob/master/dmcontent/questions.py#L369)

Your new examples take a different approach of taking a dict of data but only returning data for that question e.g.

def unformat_data(self, data):
          return {self.id: data[self.id]}

Assuming consistency is desired, raises 2 questions for me:

Should unformat_data only take input related to it's own question id or take any dict of data?
If it should take any dict of data as input, should unformat_data return output only related to it's own question or return all data with it's question unformatted?

benvand · 2017-04-12T15:31:42Z

what we want unformat_data to do.

I mean unformat_data can do whatever we want. But the reasoning behind how it functions now is that it can take all the data from a form into a question and unformat only that data that is relevant to the question.

Should unformat_data only take input related to it's own question id or take any dict of data?

We can't know what fields are relevant to a question before the data goes in so we can't only pass in the relevant fields to be unformatted.

If it should take any dict of data as input, should unformat_data return output only related to it's own question or return all data with it's question unformatted?

It comes down to the whole mutable dict thing.

Originally unformat_data would take any dictionary of data and return that data

There are 2 possibilities here. Let's say the goal of our unformat_data is to capitalize the value:

def unformat_data(self, data):
    for key in data: # iterating over a dict in python only iterates on keys
        if key == self.id:
            data[key] = data[key].capitalize()
    return data

or

# Dict comp version
def unformat_data(self, data):
    return {key: (value.capitalize() if key==self.id else value) for key, value in data.items()}

# For loop version
def unformat_data(self, data):
    d = {}
    for key in data:
        if key == self.id:
            d[key] = data[key].capitalize()
        else:
            d[key] = data[key]

The first mutates the given dictionary, the second creates a new one.
Mutating a dictionary inside a method is an easy way to introduce bugs that are really hard to debug. It's a bit too hidden and under the bonnet for me. I like my methods to explicitly return the values I want to use.

The second (the way it's done now) creates a new dictionary containing all the data

return all data with it's question unformatted

The problem arises with the second one when we start looping over questions.

d={}
for question in section.questions:
    d.update(question.unformat_data(data)) # overwrites unformatted data over and over per field

it's less valuable to define the new dictionary inside the method because you then lose the knowledge of which fields refer to a given question.

So my way:

def unformat_data(self, data):
    this_questions_data_only =  {}
    for key in data:
        if key == self.id:
            this_questions_data_only(key: data[key].capitalize())
    return this_questions_data_only

Avoids mutation, refers only to the given question, does not return data that may already have been unformatted.
And means that we can construct the dictionary outside the method like so:

d={}
for question in section.questions:
    d.update(question.unformat_data(data)) # DOES NOT overwrite unformatted data over and over per field

with no side effects.

idavidmcdonald · 2017-04-13T09:52:36Z

Cool. I think I like your argument (thank's for taking the time to write it out clearly).

It seems sensible to go with your suggestion but we should probably make unformat_data on other Questions follow the same behaviour of ditching unrelated keys (https://github.com/alphagov/digitalmarketplace-content-loader/blob/master/dmcontent/questions.py#L387). It's worth taking a look first through the content loader / front end apps to make sure there is not a dependancy of our frontend to return unrelated keys.

@allait If you've got a minute, would it be possible just for a quick check on our plans for unformat_data to make sure we aren't missing anything?

allait

Left a comment on the pricing unformat_data, but overall this seems sensible and closer to how we delegate other things between section / multiquestion and questions.

It might be worth revisiting the DynamicList implementation and updating it with the new approach (as it seems to look through the data for it's own key at the moment) and since it now should be called from the section.unformat_data we can make this a breaking change if we need to and replace the DynamicList.unformat_data calls in the apps with section.unformat_data.

allait · 2017-04-13T14:49:41Z

dmcontent/questions.py

@@ -464,6 +467,9 @@ def get_question(self, field_name):
        if self.id == field_name or field_name in self.fields.values():
            return self

+    def unformat_data(self, data):
+        return self.get_data(data)


I think this part is interesting. These 2 lines are confusing because unformat_data and get_data perform opposite operations, so they can't have the same implementation.

The reason we need this with the proposed implementation of ContentSection.unformat_data is that it delegates based on question.get_question and Pricing returns itself whenever any of it's fields are passed in. Which means that if I understand this correctly this method will get called N times for a pricing question with N fields and each time the return value will overwrite all pricing fields in the section's unformatted data dictionary. Which is not a problem because this return value is always the same.

However, this seems to suggest that we might simplify this by passing in the original field key to the quesiton.unformat_data(self, data, key) - which then allows us to use the base class implementation and return {key: data[key]}.

Although the same can be said about get_data and maybe having a similar interface between two methods makes sense.

On the other hand, they iterate over different things: get_data iterates over questions and unformat_data iterates over data keys.

benvand · 2017-04-18T17:57:37Z

@allait I believe the last 2 commits hsould address your comments. I've tried to clean up some of the code and make it clearer with comments what's going on. This has a version bump so do I need to update the changelog with the changes to unformat data on all questions?

idavidmcdonald

Looks good Ben!

One or two whitespace type comments and a question about data not existing but in principle this looks real good.

Just those tiny things to address and I know there is currently a failing Travis test to fix too.

After that it will just be the version bump. Technically we are changing the interface for DynamicList.unformat_data so it would be a breaking change... however I'm not sure if our apps actually rely on it so we might not need to change any code. So I'm not sure if it should be major or minor...

idavidmcdonald · 2017-04-19T07:52:29Z

dmcontent/content_loader.py

@@ -11,7 +11,7 @@
 from werkzeug.datastructures import ImmutableMultiDict

 from .errors import ContentNotFoundError, QuestionNotFoundError
-from .questions import Question, ContentQuestion
+from .questions import Question, ContentQuestion, Date


Is Date potentially an unused import?

idavidmcdonald · 2017-04-19T07:55:00Z

dmcontent/questions.py

-            "evidence-0": "Yes, I did."
-            "nonDynamicListKey": 'other data'
-        }
+            "evidence-0": "Yes, I did."        }


Weird bit of whitespace in here

idavidmcdonald · 2017-04-19T07:58:51Z

dmcontent/questions.py

+            root, index = question.id.split('-')
+            question_data = data[self.id][int(index)]
+            if root in question_data:
+                result.update({question.id: question_data.get(root)})


Nice, this method reads fairly easily.

Out of interest, is there a particular reason to use .update rather than result[question.id] as before or is this just a personal preference thing?

So one of the reasons I use update is to avoid the similar syntax when updating a list. Means that at a glace I can tell that result is a dict.

>>> x=[1, 2, 3, 4] >>> y=2 >>> z=3 >>> x[y] = z >>> x={1:2, 3:4} >>> x[y]=z # With the above x can be list or dict. # With the below it can only be a dict >>> x.update({y: z})

Another is that I like to use it like so especially in test fixtures:

def get_fake_brief(**kwargs): data = { 'id':1, 'lastChangedDate': '00:00:00.00000', 'frameworkSlug': 'digital-outcomes-and-specialists' ... } data.update(kwargs) return data brief1 = get_fake_brief() brief2 = get_fake_brief(id=2) brief3 = get_fake_brief(id=3, frameworkSlug='digital-outcomes-and-specialists-2')

Finally in multiple updates it avoids a for loop:

my_nonsense_dict = {} my_updates = {'foo': 'bar', 'baz': 'quux'} for key in my_updates: my_nonsense_dict[key] = my_updates[key] my_nonsense_dict = {} my_updates = {'foo': 'bar', 'baz': 'quux'} my_nonsense_dict.update(my_updates)

So yeah it's kind of personal preference but founded on the fact that if I want to do the above things and keep my code consistent then I have to use .update everywhere.

idavidmcdonald · 2017-04-19T08:38:46Z

dmcontent/questions.py

@@ -112,7 +115,7 @@ def get_error_messages(self, errors, question_descriptor_from="label"):
        return question_errors

    def unformat_data(self, data):
-        return data
+        return {self.id: data[self.id]}


Should this use .get by default in case the key does not exist in data?

For some routes, we call unformat_data on a question directly (https://github.com/alphagov/digitalmarketplace-supplier-frontend/blob/master/app/main/views/briefs.py#L202) and the data might not yet exist.

benvand · 2017-04-19T12:09:38Z

OK, should be good to go @idavidmcdonald
Version bumped and changelog updated

idavidmcdonald · 2017-04-19T12:46:46Z

CHANGELOG.md

+
+### What changed
+
+New question type `Date` and non-backwards compatible change to `Question.unformat_data` which not returns only data


typo "not" should be "now" I assume

idavidmcdonald · 2017-04-19T12:50:17Z

There is a single typo that might be nice to fix but apart from that awesome work!
👍 👍 👍 👍 👍 👍 👍
💯 💯 💯 💯 💯 💯 💯
💯 💯 💯 💯 💯 💯 💯
👍 👍 👍 👍 👍 👍 👍

benvand and others added 10 commits April 3, 2017 15:11

Add test for date summary formatting

fc68dab

wip date tests

3ba2e4e

Remove redundant tests

aead0f9

Avoid redundant checks for None

2c1aa52

wip date tests

7753666

Remove redundant tests

dcefe3f

Return None from unformat_data when any field is missing

e9d4db2

Bring datefield display inline with dm convention

a5bc981

Ignore all venvs in pep, update tests

ea3ac84

idavidmcdonald reviewed Apr 5, 2017

View reviewed changes

benvand added 2 commits April 5, 2017 22:29

Return incorrect values on error to repopulate field.

c9e96d2

Use dmutils date formats, add dmutils as dependency.

7d7502b

benvand force-pushed the 141882981-Add-question-type-to-content-loader-and-summary-display branch 2 times, most recently from 8f78029 to 0463711 Compare April 5, 2017 22:16

Refactor Section.unformat_data to be part of question

f687cd5

benvand force-pushed the 141882981-Add-question-type-to-content-loader-and-summary-display branch from 0463711 to f687cd5 Compare April 7, 2017 10:20

idavidmcdonald reviewed Apr 7, 2017

View reviewed changes

dmcontent/questions.py Outdated

FIELDS = ('year', 'month', 'day')

@property

Copy link

Contributor

idavidmcdonald Apr 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here, is this now redundant?

idavidmcdonald reviewed Apr 7, 2017

View reviewed changes

idavidmcdonald suggested changes Apr 7, 2017

View reviewed changes

benvand added 6 commits April 10, 2017 14:03

Remove redundant is_date methods on Question and Section

43a8424

Extrapolate value processing into static method

19f0a52

More tests, pass key to unformat value for assurance qs

958d24a

Linter caught redifined test

ebb7e95

Update doco line on changed unformat method

fc377ed

Remove extranous apostrophies

9294be4

benvand added 4 commits April 10, 2017 15:59

Replace setup.py dep with requirements dep

1ac9929

Version bump to 3.7.0

359618c

Fix test for python3, assert correct dates

85a23f2

idavidmcdonald reviewed Apr 11, 2017

View reviewed changes

benvand added 4 commits April 11, 2017 15:32

Retail standard unformat_data signature for list questions.

26da03e

Remove extraneous space.

d221162

Fix comment.

9165547

Remove extraneous space

1ed993c

allait reviewed Apr 13, 2017

View reviewed changes

Better comments and cleanup of Pricing get_data and unformat_data

666e986

benvand force-pushed the 141882981-Add-question-type-to-content-loader-and-summary-display branch from 92b681e to 5823b10 Compare April 18, 2017 17:59

idavidmcdonald reviewed Apr 19, 2017

View reviewed changes

benvand force-pushed the 141882981-Add-question-type-to-content-loader-and-summary-display branch from 5823b10 to bd77101 Compare April 19, 2017 10:12

Update DynamicList.unformat_data and test

c2f8b4d

benvand force-pushed the 141882981-Add-question-type-to-content-loader-and-summary-display branch from bd77101 to c2f8b4d Compare April 19, 2017 10:39

idavidmcdonald reviewed Apr 19, 2017

View reviewed changes

idavidmcdonald approved these changes Apr 19, 2017

View reviewed changes

Version bump to 4.0.0 and changelog description.

c5dae54

benvand force-pushed the 141882981-Add-question-type-to-content-loader-and-summary-display branch from 9a2c104 to c5dae54 Compare April 19, 2017 12:57

benvand merged commit 8ddf063 into master Apr 19, 2017

		def unformat_data(self, data):

		"""Unpack assurance information to be used in a form


		return {self.id: '-'.join(parts) if any(parts) else None}

		def unformat_data(self, data):


		### What changed

		New question type `Date` and non-backwards compatible change to `Question.unformat_data` which not returns only data

141882981-Add-question-type-to-content-loader-and-summary-display #36

141882981-Add-question-type-to-content-loader-and-summary-display #36

Conversation

benvand commented Apr 4, 2017 • edited Loading

Create handlers for processing and displaying date questions.

idavidmcdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idavidmcdonald left a comment • edited Loading

Choose a reason for hiding this comment

idavidmcdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idavidmcdonald commented Apr 12, 2017

benvand commented Apr 12, 2017 • edited Loading

idavidmcdonald commented Apr 13, 2017

allait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allait Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

benvand commented Apr 18, 2017

idavidmcdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvand commented Apr 19, 2017

Choose a reason for hiding this comment

idavidmcdonald commented Apr 19, 2017

benvand commented Apr 4, 2017 •

edited

Loading

idavidmcdonald left a comment •

edited

Loading

benvand commented Apr 12, 2017 •

edited

Loading

allait Apr 13, 2017 •

edited

Loading